Skip to content

add read/writePointer to be used in threads (de)serializePointer#804

Open
BTNC wants to merge 2 commits into
torch:masterfrom
BTNC:rwp
Open

add read/writePointer to be used in threads (de)serializePointer#804
BTNC wants to merge 2 commits into
torch:masterfrom
BTNC:rwp

Conversation

@BTNC

@BTNC BTNC commented Oct 19, 2016

Copy link
Copy Markdown
Contributor

Hi,

Added read/writePointer together with a test case 'readWritePointer' in test/test_sharedmem.lua. The implementation of (de)serializePointer in threads will be replaced with these new functions instead of relying on readWriteLong/Double.

Travis-ci fail for osx with following error during test, but the compilation does succeed.
/Users/travis/torch/install/bin/luajit: /Users/travis/torch/install/share/lua/5.1/torch/init.lua:13: cannot load '/Users/travis/torch/install/lib/lua/5.1/libtorch.so'

Thanks,

@soumith

soumith commented Oct 19, 2016

Copy link
Copy Markdown
Member

thanks. dont worry about the OSX build, it's been failing on travis, i have to take a look at it.

@gchanan gchanan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to declare this for all File types, or can it just be in MemoryFile? Seems only useful there.

I haven't thought through if this complicates the implementation; have you considered it?

Comment thread doc/file.md
equals to the size of the given storage, and fill up the storage with these elements.
The number of elements actually read is returned.

A convenient method exists to read one pointer as a integer: `[integer] readPointer()`. It reads

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename convenient to convenience.

Comment thread doc/file.md

These methods return the number of elements actually written.

A convenient method exists to write one pointer: `writePointer(integer)`. It writes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too.

@BTNC

BTNC commented Mar 25, 2017

Copy link
Copy Markdown
Contributor Author

Yes, it seems only useful for MemoryFile. However if remove this from DiskFile, it would be against the purpose of virtual table and will complicate the implementation and maintenance. By using virtual table (as in c++), it is not expected some sub class have null function pointers in virtual table. To remove this from DiskFile, the function pointers have to be initialized as null pointers and the callers have to check if the function pointers are null pointers before calling them. This special logic will make the implementation less straightforward and less maintainable. Besides, it will add unnecessary null pointer check for the normal case, the MemoryFile case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants